-
Notifications
You must be signed in to change notification settings - Fork 850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Force IPv4 resolving #1046
Force IPv4 resolving #1046
Conversation
LGTM, a minor version I think? |
Either patch or minor, no strong feelings 🤷 |
This change breaks my server. Though you made this change in Nov 2020, I am just now trying upgrading from 6.15 to 7.122, and while it works on my test server it doesn't on production. My production server is IPv6 only. That doesn't mean it can't make or receive IPv4 requests, but it is behind proxies. Incoming IPv4 requests are proxied as you'd expect, and outgoing requests, which is the case here, are also proxied. But because the server is IPv6 only, it has to be able to talk to the proxy over IPv6, and the CURL_IPRESOLVE_V4 prevents that. The comment in the code says "Stripe's API servers are only accessible over IPv4". That does not appear to be true. If I do the opposite (CURL_IPRESOLVE_V6 - which would obviously break IPv4 only servers), then I can make requests and make a payment satisfactorily. api.stripe.com has IPv6 AAAA records, and responds properly to testing. (I think REKE6M-8KLLR is a ticket number in Stripe support which I raised about this, but having now tracked down the problem, it made sense to comment here). |
Hello @davidearl, thank you for the report! It will take us some time to investigate this properly and come up with a recommendation, but I'll try and check back with you next week! |
Hey @davidearl , just following up here. The Stripe API no longer has an AAAA record and does not currently support IPv6. For your use case, we could potentially expose a toggleable setting on the |
Thank you, yes that would work, but only providing a possible setting is NULL. Setting it explicitly to IPv6 won't work if there are no AAAA records (but see below), and setting it explicitly to IPv4 (which is how you have it currently) won't work because it needs to start the journey over IPv6 to the proxy. With NULL (or just not making that option call) it works whether or not IPv6 is supported, as it is equivalent to not having that line at all, which is how I am working currently, with that line commented out. Indeed, knowing what the problem is, I'm happy just to continue to comment it out: updating the API isn't done frequently. I had another look at your IPv6, and while it is true that a check at mxtoolbox.com does not yield a IPv6 address, my server continues to do so! Presumably you have only recently removed it and it is cached somewhere between my server and the master DNS (though why the TTL hasn't thrown it out by now is odd). Furthermore, those IPv6 addresses also work! (it would be a problem they didn't, as it would get a IPv6 address from the cache, attempt to use it, and fail). I'm curious why you nominally withdrew IPv6 when it patently works.
|
Thanks for the great feedback @davidearl . I've sent #1272 to let you pass in a custom option for IPRESOLVE. You could now instantiate your
This will allow using both IPv4 and IPv6 for resolving.
As far as I know, this is still a work in progress but is not yet supported hence why the AAAA record was removed. I've double checked with the team though that it is not currently supported. |
For anyone reading this in the future, once you have the curl client, you need to tell Stripe to use it thus (see their README):
|
I am affected by this. Can this be reverted, the original issue #1045 mostly likely had a broken proxy that probably affected his whole network(not only stripe). Even in the future when stripe adds IPv6 support, happy eyeballs will fix any user issues. This breaks IPv6 only setups and DNS64 setups such as @davidearl currently using. |
Update: In the latest version of the stripe-php library, we no longer force resolution to IPV4 addresses. Forcing IPv4 was causing hard-to-understand failures for users in IPv6-only environments. If you want to force IPv4 yourself, you can do so by telling the API client to use a CurlClient other than the default
|
r? @richardm-stripe
Had a few minutes to kill while waiting for a merge :)
The constants are defined in curl 7.10.8 which is 17 years old. I've verified that the oldest curl version we see in live traffic is 7.19.7 (still a respectable 11 years old) so I don't think we need to add
\defined
guards or manually define the constants ourselves.Fixes #1045.